I initially introduced the notion of a 'terminal' op that is fusable only at the end of a window. I don't use this so will remove it.

As stated, the above constraint isn't particularly cheap....and it doesn't seem to ever be "used" I will look into the perf, but I suspect the get_projection method is the culprit here

fusion constraints are registered above

the above func will be cleaned up and simplified in the next day or so

If the store already has a key partition (set by the fuser), no need to compute it
There may be a more elegant way of doing this

You should check if the key partition satisfies the restrictions before you reuse it. And I think this logic should be moved to compute_key_partition. Just so you know, I'm currently refactoring the code that manages legate stores, so don't make any micro optimization like this, as that will become obsolete in the near future.

I will come back to this; this was done to make constant optimization work; I will check if I indeed need this assertion removed/modified

all changes to this file will be reverted before merge

Is the current version indeed 0.1? If so I'll undo the above change

I'm not a fan of this design. The fusion task shouldn't be any different from a normal Legate task. I believe the fusion metadata can be passed as a bunch of scalar arguments to the fusion task. If we do that, then we don't need to make every place in the core handle fusion. Making the default path aware of task fusion doesn't sound like a good design.

This is def worth discussing (I pondered this myself):
I did think about making them scalars, but was not sure if there were any other implications/downsides of storing all the metadata that way (eg are there a maximum number of scalars we can give a task), since we'd be sending in quite a few scalars to the task (or rather multiple lists, each of which can have >50 scalars; the number of scalars in 4/5 of these lists is equivalent to the fused_op length, even with potential deduplication of stores).
If we did this, in the task's scalars array, we'd thus be mixing "metadata" scalars (which are really a bunch of lists) with the actual scalars used by the sub-tasks, which seemed to not really adhere to the abstraction of having a dedicated "scalars" array in context data structure. I thus chose to serialize it as Legate Task argument data. As you stated, the downside is that the core/default has to be "fusion aware".

But if you don't see any potential downsides of making them all scalars, then yeah it could totally be implemented to be scalar based. Would mostly just have to move book-keeping logic from the deserializer into the Fused Legate Task

I feel what's actually missing here is an ability to pass an arbitrary chunk of data (probably a memoryview object in Python) to a task, which in theory possible, so we should probably extend the buffer builder interface to take an an arbitrary memory view object and pack it as a single contiguous chunk. The assumption here is that the task knows how to interpret those bytes. It shouldn't be too difficult for me to add it.

Why is this constructor necessary?

Each sub-op is called like this (via function-pointer):

for (int i=0; i<fused_op_len; i++){
      auto descriptor = Core::gpuDescriptors.find(opIDs[i])->second;
      descriptor(context);
}

However, the context above is not the same as the parent tasks's context; Whereas the parent task's context contains the "union" of all input outputs reductions and scalars over all subops, the above context should only include the inputs, outputs etc for sub-operation/task i. I used this constructor to build the operation-specific contexts, for holding the specific set of inputs outputs etc.

This brings up a potential issue of that the new context's task and regions fields are currently not specific to the sub-op. I'm not sure how much of an issue that is, as I only need the context structure to hold stores and scalars for the sub-ops. Ideally I wanted to avoid any performance costs of creating of new Task objects in the Fused Task, so I elected to keep the new context object as such.

You do need a new context in this case, but my question is more on why you couldn't use the existing constructor. The constructor you added leaves the runtime and context member uninitialized, leading to a crash if the child task tries to access the runtime (though no task in cunumeric is actually doing that). I think the context you are creating here should still have a valid runtime and context pointer.

That said, I think you actually need a slightly different change here, which allows TaskContext to override the legion task's id. The task object here is of the fusion task, so if you pass it as-is to a child task, the child task may think that it's not what it thinks it is. I believe no cuNumeric task is doing actually doing it, but leaving this unaddressed can be a foot gun later on.

Yeah it's a valid point. I initially tried using the normal constructor and got runtime errors with from the task deserializer. Recreating the issue I get: ( the error is from span.h : Assertion pos < size_ failed.)

legion_python: /home/shiv1/fmultiGPU/legate.core/src/core/utilities/span.h:38: decltype(auto) legate::Span<T>::operator[](size_t) [with T = const Legion::PhysicalRegion; size_t = long unsigned int]: Assertion `pos < size_' failed.
Signal 6 received by node 0, process 3131805 (thread 7f8d47bf3000) - obtaining backtrace
Signal 6 received by process 3131805 (thread 7f8d47bf3000) at: stack trace: 19 frames
  [0] = /lib/x86_64-linux-gnu/libpthread.so.0(+0x153c0) [0x7f8d70ce33c0]
  [1] = /lib/x86_64-linux-gnu/libc.so.6(gsignal+0xcb) [0x7f8d707f718b]
  [2] = /lib/x86_64-linux-gnu/libc.so.6(abort+0x12b) [0x7f8d707d6859]
  [3] = /lib/x86_64-linux-gnu/libc.so.6(+0x25729) [0x7f8d707d6729]
  [4] = /lib/x86_64-linux-gnu/libc.so.6(+0x36f36) [0x7f8d707e7f36]
  [5] = /home/shiv1/fmultiGPU/legate.core/install/lib/liblgcore.so(+0x5a95d) [0x7f8d4428595d]
  [6] = /home/shiv1/fmultiGPU/legate.core/install/lib/liblgcore.so(legate::TaskDeserializer::_unpack(legate::Store&)+0x10e) [0x7f8d44286d9e]
  [7] = /home/shiv1/fmultiGPU/legate.core/install/lib/liblgcore.so(legate::TaskContext::TaskContext(Legion::Task const*, std::vector<Legion::PhysicalRegion, std::allocator<Legion::PhysicalRegion> > const&, Legion::Internal::TaskContext*, Legion::Runtime*)+0x7f6) [0x7f8d44278626]
  [8] = /home/shiv1/fmultiGPU/legate.core/install/lib/libcunumeric.so(cunumeric::FusedOpTask::gpu_variant(legate::TaskContext&)+0x6e2) [0x7f8d1ce0a852]

at the time I didn't spend much time debugging this and just wanted something that worked for the time being (the constructor I made didn't call the deserializer. I didn't want to reserialize/deserialize things anyways because it involves additional overhead). I'm guessing it has to do with the number futures or scalars passed in, but I can totally dive a little deeper into what's causing this

I suspect the crash is due to the fusion metadata you added that the current serializer doesn't understand, so this issue is tied with the fusion awareness introduced in the code base, which I suggest removing. Actually, we shouldn't be deserializing the task argument for the child task context, so you're right that there be a constructor that doesn't serialize the argument. I guess we could add an argument to the existing constructor to control the behavior.

Why did you want to expose the private members here?

ahh this should be reverted. This is an artifact from when we were using the inline launcher, and I needed access to these

I guess we'd eventually want coalescing of arguments here as well. Right now, the fusion code passes the same store used by multiple tasks multiple times, hoping that the launcher at least coalesces region requirements for those duplicates. But, we can identify that the same store is being used in multiple tasks when we scan them for dataflow analysis. So, we could potentially pass a deduplicated list of stores to the fusion task with a mapping between the original indices and those in the store list that is passed to the fusion task.

Yeah we could deduplicate these, and I don't imagine this being too difficult. We would just have to place the offset of the dedpulicated store (eg in line 1145) in the offsets array, and it should just work

this shouldn't be hardcoded, will remove